Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add nc4-formatted tile file when generating bcs #1028

Merged
merged 61 commits into from
Feb 21, 2025
Merged

add nc4-formatted tile file when generating bcs #1028

merged 61 commits into from
Feb 21, 2025

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Nov 5, 2024 •
edited by gmao-rreichle
Loading

This PR modifies make_bcs to create an NC4-formatted tile file, in addition to the ASCII-formatted tile files.

The NC4-formatted tile file also includes the supplemental tile attributes (min/max lat/lon and elevation of each tile) needed by GEOSldas, which are currently stored in the (land-only) "catchment.def" file. The supplemental tile attributes are now also computed for landice and lake tiles.

The PR also includes a py script and an F90 utility program to retroactively create NC4-formatted tile files for existing bcs.

Because the PR only touches make_bcs, it should be trivially 0-diff for the standard GCM and GEOSldas nightly tests.

The PR is also 0-diff for bcs generation (except for the version-specific README file generated by make_bcs).

In the future, the ASCII-formatted tile file and catchment.def file should become obsolete.

Contingencies: #909 must be merged first.

@tclune @biljanaorescanin @sdrabenh @lcandre2

@weiyuan-jiang weiyuan-jiang added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Nov 5, 2024
@gmao-rreichle
Copy link
Contributor

cc @gmao-rreichle

@gmao-rreichle gmao-rreichle changed the title added nc4 file for tile file when generating bcs add nc4-formatted tile file when generating bcs Dec 14, 2024
@weiyuan-jiang
Copy link
Contributor Author

@gmao-rreichle It is ready for your review

@gmao-rreichle
additional comments and cleanup (TileFile_ASCII_to_nc4.F90, mkCatchPa…
19eb1cb 
…ram.F90, mkEASETilesParam.F90, rmTinyCatchParaMod.F90)
@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang: To compute the supplemental attributes for landice and lake tiles, I suspect we need to modify the (renamed) subroutine supplemental_tile_attributes(). Specifically, I think the indices ip1 and ip2 appear to constrain the calculations to just land tiles. Perhaps it would not be difficult to calculate the supplemental attributes for landice and lake tiles. We do not want them for ocean tiles, and we do not want them written into the legacy "catchment.def" file. The idea is to have good values for min/max lat/lon and elevation of landice and lake tiles in the new, nc4-formatted tile file.

GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/rmTinyCatchParaMod.F90

Line 2138 in 19eb1cb

if((catid(i) > ip1).and.(catid(i) <= ip2))then

@weiyuan-jiang
Copy link
Contributor Author

@weiyuan-jiang: To compute the supplemental attributes for landice and lake tiles, I suspect we need to modify the (renamed) subroutine supplemental_tile_attributes(). Specifically, I think the indices ip1 and ip2 appear to constrain the calculations to just land tiles. Perhaps it would not be difficult to calculate the supplemental attributes for landice and lake tiles. We do not want them for ocean tiles, and we do not want them written into the legacy "catchment.def" file. The idea is to have good values for min/max lat/lon and elevation of landice and lake tiles in the new, nc4-formatted tile file.

GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/rmTinyCatchParaMod.F90

Line 2138 in 19eb1cb

if((catid(i) > ip1).and.(catid(i) <= ip2))then

Is the min/max of lat/lon of landice and lake calculated by the same way as land ?

@gmao-rreichle
Copy link
Contributor

Is the min/max of lat/lon of landice and lake calculated by the same way as land ?

I would think so. The information from the raster (*.rst) file, which is the basis for the min/max lat/lon calculations, isn't specific to land and should work for landice and lake in the same way.

@lcandre2
Copy link

Is the min/max of lat/lon of landice and lake calculated by the same way as land ?

I would think so. The information from the raster (*.rst) file, which is the basis for the min/max lat/lon calculations, isn't specific to land and should work for landice and lake in the same way.

I agree with Rolf. The lake and landice tile min and max should be calculated the same way as land. Like land, both are irregular.

@weiyuan-jiang
Copy link
Contributor Author

Is the min/max of lat/lon of landice and lake calculated by the same way as land ?

I would think so. The information from the raster (*.rst) file, which is the basis for the min/max lat/lon calculations, isn't specific to land and should work for landice and lake in the same way.

I agree with Rolf. The lake and landice tile min and max should be calculated the same way as land. Like land, both are irregular.

OK, I will take care of it

@biljanaorescanin biljanaorescanin requested a review from a team as a code owner January 31, 2025 13:09
@biljanaorescanin
Copy link
Contributor

Testing summary:
1. PR was zero diff for BCS generation functionality.
I've run C180, EASE M09, C12 MOM6 and CF0270x6C-SG001 to confirm zero diff.
Only difference was as expected in README file. (This difference is really a README documentation bug fix. )
2. PR was zero diff for GEOSldas regression testing.
3. PR was zero diff for 1-day AMIP, REPLAY and Inc REPLAY.

@sdrabenh this PR is also ready for you. To be merged you must first merge #909. That is why I am leaving blocking label.
My v13 BCS PR will now come on top of this PR.

gmao-rreichle
gmao-rreichle previously approved these changes Jan 31, 2025
View reviewed changes
lcandre2
lcandre2 previously approved these changes Jan 31, 2025
View reviewed changes
@biljanaorescanin biljanaorescanin mentioned this pull request Jan 31, 2025
Merged
@gmao-rreichle gmao-rreichle marked this pull request as draft February 11, 2025 21:57
@biljanaorescanin
Copy link
Contributor

We fixed all the labels so we are good to be merged.

@biljanaorescanin biljanaorescanin marked this pull request as ready for review February 14, 2025 19:48
sdrabenh
sdrabenh previously approved these changes Feb 21, 2025
View reviewed changes
bena-nasa
bena-nasa previously approved these changes Feb 21, 2025
View reviewed changes
@weiyuan-jiang weiyuan-jiang dismissed stale reviews from bena-nasa, sdrabenh, and biljanaorescanin February 21, 2025 16:22

The merge-base changed after approval.

@biljanaorescanin biljanaorescanin removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Feb 21, 2025
@sdrabenh sdrabenh merged commit 4c94e7e into develop Feb 21, 2025
5 of 9 checks passed
@sdrabenh sdrabenh deleted the feature/wjiang/nc4_tilefile branch February 21, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathomp4 mathomp4 mathomp4 approved these changes

@biljanaorescanin biljanaorescanin biljanaorescanin approved these changes

@bena-nasa bena-nasa bena-nasa left review comments

@lcandre2 lcandre2 lcandre2 left review comments

@sdrabenh sdrabenh sdrabenh left review comments

@gmao-rreichle gmao-rreichle gmao-rreichle left review comments

Assignees

@biljanaorescanin biljanaorescanin

Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@weiyuan-jiang @gmao-rreichle @lcandre2 @atrayano @biljanaorescanin @mathomp4 @bena-nasa @sdrabenh